Skip to content

Display links to things in local systems #1291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 30 commits into from
May 27, 2025

Conversation

lrosenstrom
Copy link
Contributor

@lrosenstrom lrosenstrom commented May 9, 2025

Description

Tickets involved

LWS-249

Solves

The goal is to provide access to items in the local library catalog as well as additional library info such as address, opening hours etc. This is currently presented in the holding panel:

Screenshot from 2025-05-22 09-17-55

Summary of changes

  • Construct a mapping for library code (sigel) to the links/info on fnurgel page load
  • Tried to keep close to the Figma sketches for the frontend part
  • Add a basic server side caching mechanism for full holder/library data. There is plenty of information about caching in the browser in a SvelteKit context but not so much about caching on the server. The approach used in this PR comes from https://www.loopwerk.io/articles/2025/svelte-5-stores/, which should be fine since the data we are caching is not tied to a specific user/session/browser. I think introducing an in-memory database such as Redis is overkill for this, but that's what most people seem to be suggesting in the forums.

@lrosenstrom lrosenstrom force-pushed the feature/lws-249-link-to-holding branch from d866317 to 1f939ce Compare May 12, 2025 13:37
@lrosenstrom lrosenstrom marked this pull request as ready for review May 22, 2025 07:39
Copy link
Contributor

@johanbissemattsson johanbissemattsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🙌

We probably want some kind of support for refreshed cache data in the future (so updated libraries data will be reflected in the cache without having to restart the server) but this can maybe be saved for later? I think it would be good to have a comment or issue created regarding this if the question arises.

Copy link
Contributor

@olovy olovy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

See comments.

As discussed offline:

  • The key for holders/libraries should be @id not sigel
  • records should be referenced by @id, not controlNumber. controlNumber is a MARC identifier and can contain old numeric Libris IDs. If systemIds/fnurgels are preferred because they are shorter in the URL they should be constructed by removing the system base URI prefix.

This behavior hasn't changed in this PR and can be fixed later.

Comment on lines 207 to 246
export async function getFullHolderData(allHolders: DecoratedHolder[]): Promise<FullHolderBySigel> {
const holderBySigel: FullHolderBySigel = {};

for (const h of allHolders) {
const id = h.obj?.['@id'];
const libraryRes = await fetch(`${id}?framed=true`, {
headers: { Accept: 'application/ld+json' }
});
const resJson = await libraryRes.json();
const libraryMainEntity = resJson['mainEntity'] as FramedData;

if (libraryMainEntity) {
holderBySigel[h.sigel] = libraryMainEntity;
}
}
return holderBySigel;
}

export async function fetchHoldersIfAbsent(holdersByType: HoldersByType) {
const cachedHolders = holdersCache.holders;
const allHolders = Object.values(holdersByType).flat();
for (const h of allHolders) {
const id = h.obj?.['@id'];

if (h.sigel && cachedHolders && !cachedHolders[h.sigel]) {
const response = await fetch(`${id}?framed=true`, {
headers: { Accept: 'application/ld+json' }
});
if (response.ok) {
const resJson = await response.json();
const libraryMainEntity = resJson['mainEntity'] as FramedData;
if (libraryMainEntity) {
cachedHolders[h.sigel] = libraryMainEntity;
}
} else {
console.error(`Could not fetch holder data for ${id}`);
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be the same fetch?
error handling missing in the first version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first function is unused and was probably used before adding the cache+error handling. Must have missed to delete it! Good catch


const linkTemplateEod = getAtPath(fullHolderData, [BibDb.eodUri], []);
if (linkTemplateEod && linkTemplateEod.length !== 0) {
linksToItem = [linkTemplateEod.replace(/%BIB_*ID%/, bibIdObj.bibId), ...linksToItem];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will only replace the first occurence of %BIB_*ID%
same comment for all replace

</li>
{/if}
{#if linksByBibId[firstBibId]?.[holder.sigel]?.[BibDb.Address]}
{#each linksByBibId[firstBibId][holder.sigel][BibDb.Address] as address (address)}
Copy link
Contributor

@jesperengstrom jesperengstrom May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't use adress as key here because it's sometimes missing...

@jesperengstrom
Copy link
Contributor

Nice work!

Apart from comment that breaks the app for me, I mainly have comments regarding the look-and-feel which can be iterated. For example...

Skärmavbild 2025-05-23 kl  14 12 32
  • I think this blue button is too wide :)
  • I think we need some visual hierarchy with so much information. For example: indenting the opening hours a bit so it's visually a child of the summary label.
  • Shouldn't we check if there are opening hours earlier and skip this section if it's missing? (lots of empty 'opening hours')

@lrosenstrom
Copy link
Contributor Author

Put item availability together with the rest of the item/instance info and made some improvements to the layout

Screenshot from 2025-05-26 14-40-15

@lrosenstrom lrosenstrom merged commit fff5d60 into develop May 27, 2025
2 checks passed
@lrosenstrom lrosenstrom deleted the feature/lws-249-link-to-holding branch May 27, 2025 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants